Add support for property initialization during cloning #6538

kocsismate · 2020-12-26T02:08:01Z

The primary purpose of this RFC is to make cloning less verbose when multiple properties are assigned to afterwards, but it might be useful in case of initonly properties in the future.

Given the following piece of code:

class Foo
{
    public $bar;
    public $baz;

    public function withProperties($bar, $baz)
    {
        $self = clone $this;
        $self->bar = $bar;
        $self->baz = $baz;

        return $self;
    }
}

This RFC would allow to write:

class Foo
{
    public $bar;
    public $baz;

    public function withProperties($bar, $baz)
    {
        return clone $this with {
            bar: $bar,
            baz: $baz,
        };
    }
}

iluuu1994

Tests are a little sparse 🙂 Some things I could think of:

Test that assigning the wrong type will trigger a type error
Test accessibility modifiers still work
Test that it works from outside the class (if that is intended)

Another issue is that publicly exposed properties are very rare in the PHP ecosystem at the moment because we do not have asymmetric visibility. For any property that requires calling a setter this feature will not be usable.

Full disclosure, I'm not sure I'm in support for this RFC if we solely introduce it for initonly as (IMO) it seems like a solution to a non-problem. initonly is a locked property, if it needs to change, from the inside or outside, asymmetric visibility is more fitting. We don't have that yet but that doesn't mean we should make initonly do stuff it's not meant to do.

Crell · 2020-12-26T16:27:55Z

I'm of decidedly mixed feelings here.

This gives us a single-expression "with-er" method. I like.
It would therefore dovetail nicely with short-functions. I like.
It gives us a struct-like immutable syntax similar to that found in Rust et al. I don't think we could reasonably do better than this for native "Evolvable" support. I like.
It's more compact than a separate "with" method for each property, and more performant, but it looks like it would still be chainable? (clone $foo with {x: $newX }->stuff();)
It would only play nice with initonly if cloning like this is considered an "init" context, and thus writeable. Without that, they'd be incompatible and thus not useful.
I am extremely skeptical about allowing "with undefined property". IMO it should be limited to defined properties, which are superior in basically every conceivable way.
As Ilija notes, there's scoping questions galore. If the properties are private, then you still need getters for any of them anyway, as now. If they're public, or public initonly, then there's no way to guarantee relationships between properties. (Think $x, $y, and $area.) Only allowing the with clause internally within the class is an arbitrary and non-intuitive restriction.

Point 7 is the really big one, as it means you either have to risk invalid state, or build methods around everything to the point that you wouldn't get much benefit anyway.

On its own, if it followed scoping rules on private/protected (you can't clone-with a private property from outside the object, but you can within the object), it would make with-er ethods easier and single-expression, thus making them compatible with short-functions. Those are all wins in my book, so I'm in favor generally. It's initonly that I don't think is going to be sufficient, and this would be better paired with asymmetric visibility to get the most benefit out of it.

The alternative here would be to allow passing potentially named properties to the __clone() method. That would be more flexible, but require more code to achieve this use case (which I expect to be the most common).

kocsismate · 2020-12-27T12:30:45Z

Tests are a little sparse 🙂 Some things I could think of:

Yeah, that's just because it was extremely late when I finished the initial implementation. And I didn't want to put even more effort into the PR yet, until I hear some initial feedback. But nevertheless, I also planned to add such tests! P.S. I've just added a test locally if visibility rules apply, and it worked well (as originally imagined). :)

It's more compact than a separate "with" method for each property, and more performant, but it looks like it would still be chainable? (clone $foo with {x: $newX }->stuff();)

Yes, I think it's chainable, the as far as I saw with {} doesn't change this fact. Nevertheless, I'll add a test.

It would only play nice with initonly if cloning like this is considered an "init" context, and thus writeable. Without that, they'd be incompatible and thus not useful.

Yeah, my intention is to make "clone with" a special case when I propose initonly.

I am extremely skeptical about allowing "with undefined property". IMO it should be limited to defined properties, which are superior in basically every conceivable way.

In general, I also agree with the superiority of defined properties, but I'm not sure if forbidding dynamic properties should be in the scope of "clone with". Nevertheless, we can discuss it with a broader audience (e.g. on list or in chat).

It's initonly that I don't think is going to be sufficient, and this would be better paired with asymmetric visibility to get the most benefit out of it.

That's ok, and let's talk about it when discussing the initonly and the asymmetric visibility features.

The alternative here would be to allow passing potentially named properties to the __clone() method. That would be more flexible, but require more code to achieve this use case (which I expect to be the most common).

This is an interesting approach, but I'm not sure it would work well in practice. My main concern - besides that it requires lots of boilerplate, basically a second constructor - is that you can't (currently) distinguish a nullable property from a property which you don't want to initialize after cloning. Besides, I'm not sure how the parameter list of this method would look like? Or this would be an exempt from variance rules (just like a constructor)?

TysonAndre

Not sure how I feel about this - it'd be useful with initonly properties.

Without initonly properties, it's convenient syntactic sugar, but may not be used often in practice

TysonAndre · 2020-12-27T16:10:31Z

Zend/zend_vm_def.h

+		ZVAL_DEREF(value);
+	}
+
+	if (OP2_TYPE == IS_CONST) {


It looks like OP2_TYPE is always const in the code generated by zend_compile.c? Is extending this to dynamic property names an open question?

Hmm, it shouldn't be, only OP1 should be always a constant. I've just added two tests to support my claims :) They'll ensure that op1 is a const, and op2 can be an expression.

TysonAndre · 2020-12-27T16:11:36Z

Zend/zend_ast.c

@@ -2079,6 +2084,12 @@ static ZEND_COLD void zend_ast_export_ex(smart_str *str, zend_ast *ast, int prio
 			ast = ast->child[1];
 			goto tail_call;

+		case ZEND_AST_INITIALIZER_EXPR:
+			smart_str_append(str, zend_ast_get_str(ast->child[0]));
+			smart_str_appends(str, "= ");


Can merge with ZEND_AST_NAMED_ARG

Suggested change

smart_str_appends(str, "= ");

smart_str_appends(str, ": ");

Thanks for spotting this! I originally used = in the initializer expression syntax, so this one stuck here from that time.

TysonAndre · 2020-12-27T16:13:15Z

ext/tokenizer/tokenizer_data.c

@@ -86,10 +37,25 @@ void tokenizer_register_constants(INIT_FUNC_ARGS) {
 	REGISTER_LONG_CONSTANT("T_CONSTANT_ENCAPSED_STRING", T_CONSTANT_ENCAPSED_STRING, CONST_CS | CONST_PERSISTENT);
 	REGISTER_LONG_CONSTANT("T_STRING_VARNAME", T_STRING_VARNAME, CONST_CS | CONST_PERSISTENT);
 	REGISTER_LONG_CONSTANT("T_NUM_STRING", T_NUM_STRING, CONST_CS | CONST_PERSISTENT);
+	REGISTER_LONG_CONSTANT("T_INCLUDE", T_INCLUDE, CONST_CS | CONST_PERSISTENT);


What's the new sorting order based on?

I just run the tokenizer_data_gen.sh script ^^ as I thought that everyone does this, but then I realized there are too many changes... That said, I'm ok to revert this if this script shouldn't be used. :)

I think someone may have changed the order of the tokenizer constants without being aware of this script. It may be useful to run that script in a separate PR anyway if we plan to keep using it.

I didn't notice that that file was created by a script

I think they may have been reordered in b03cafd which didn't update tokenizer_data.c

I'll create a separate PR for claiming back the original token order.

TysonAndre · 2020-12-27T16:20:32Z

Zend/zend_vm_def.h

@@ -1095,6 +1095,150 @@ ZEND_VM_C_LABEL(assign_op_object):
 	ZEND_VM_NEXT_OPCODE_EX(1, 2);
 }

+ZEND_VM_HANDLER(200, ZEND_INIT_OBJ, VAR, CONST, CACHE_SLOT, SPEC(OP_DATA=CONST|TMP|VAR|CV))


This seems to only support clone $variable with {...}.

I assume there's more work planned to support clone function_result() with {...} (TMP/TMPVAR)

Actually, I wasn't aware of this, since it's my first time when I touch the VM. Yeah, function_result() should work as well, so I'll need to work on this for sure.

Ok, so I've just tested (and just pushed the change) that the clone operand can be an expression. Did you say so because of the VAR op1? This is really something I'll have experiment more with. originally, I used TMPVAR, but that didn't work (maybe because of unrelated reasons).

TysonAndre · 2020-12-27T16:23:43Z

Zend/zend_vm_def.h

+				} else {
+ZEND_VM_C_LABEL(fast_assign_obj):
+					value = zend_assign_to_variable(property_val, value, OP_DATA_TYPE, EX_USES_STRICT_TYPES());
+					if (UNEXPECTED(RETURN_VALUE_USED(opline))) {


When/how would the return value be used? This seems to be for prop_name = expr when the property is untyped

For now, I only copy-pasted the handler of ZEND_ASSIGN_OBJ and modified it only as much as necessary. So I'll definitely want to get rid of the unused parts a bit later.

Zend/zend_compile.c

TysonAndre · 2020-12-27T16:49:43Z

Zend/tests/clone_initializer/clone_initializer_success2.phpt

+    {
+        return clone $this with {
+            property1: 1,
+            property2: "foo",


Could add a test of this with function calls, e.g.

If a previous property expression throws, subsequent ones are not evaluated?

If an expression throws, type checks are not performed (e.g. clone $this with { strTypedProperty: throw new Exception("test") })

temporary values such as strtolower("ABC") are freed if there's a type error assigning to a typed property

EDIT: Oh. So far, this is only implemented for constant operands in Zend/zend_vm_def.h

Thanks for the ideas, I'll definitely add such tests!

kocsismate · 2020-12-27T17:39:58Z

Without initonly properties, it's convenient syntactic sugar, but may not be used often in practice

I agree that this construct would be the most useful for initonly, but I think it still has its use-case currently. Let's just think about PSR-7 (e.g. https://github.com/laminas/laminas-diactoros/blob/2.6.x/src/ServerRequest.php#L128). But I believe the majority of immutable objects would benefit from this feature.

iluuu1994 · 2022-04-17T22:45:08Z

@kocsismate Same here right? 🙂

kocsismate · 2022-04-18T05:53:31Z

Yeah, I'd like to try to finish this as well, although the situation is a bit worse here as I don't know what the best approach would be to implement this.

kocsismate · 2022-10-06T11:39:00Z

Closing in favor of #9497

kocsismate added the RFC label Dec 26, 2020

iluuu1994 reviewed Dec 26, 2020

View reviewed changes

kocsismate force-pushed the clone-init branch from 8ceaa77 to dce4955 Compare December 27, 2020 12:36

TysonAndre reviewed Dec 27, 2020

View reviewed changes

kocsismate force-pushed the clone-init branch from dce4955 to 9a7a394 Compare December 27, 2020 17:23

Add support for property initialization during cloning

8928766

kocsismate force-pushed the clone-init branch from 9a7a394 to 8928766 Compare December 27, 2020 17:29

iluuu1994 added the Waiting on Author label Apr 17, 2022

kocsismate closed this Oct 6, 2022

kocsismate deleted the clone-init branch October 6, 2022 11:39

Add support for property initialization during cloning #6538

Add support for property initialization during cloning #6538

Uh oh!

Conversation

kocsismate commented Dec 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Crell commented Dec 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kocsismate commented Dec 27, 2020

Uh oh!

TysonAndre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TysonAndre Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate commented Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluuu1994 commented Apr 17, 2022

Uh oh!

kocsismate commented Apr 18, 2022

Uh oh!

kocsismate commented Oct 6, 2022

Uh oh!

Uh oh!

kocsismate commented Dec 26, 2020 •

edited

Loading

Crell commented Dec 26, 2020 •

edited

Loading

kocsismate Dec 27, 2020 •

edited

Loading

TysonAndre Dec 27, 2020 •

edited

Loading

kocsismate commented Dec 27, 2020 •

edited

Loading